Conversation
- Add kotlinx-collections-immutable
…resenter (#94) - Custom Design Support: Routed toasts through EventHandler to MainActivity to support custom UI overlays. - If reverting to system default toasts, the global EventHandler logic can be replaced with direct Toast.makeText calls within each screen's side-effect handler.
#94) - Rename SideEffectHandler (FocusingSearchToastEffect -> FocusingSearchSideEffect) - Introduced PersistentList for CircleModel to optimize recomposition performance.
- Migrated toast logic to the new SideEffect system across nlsearch, person, and photoDetil modules.
- Implement image share functionality using side effects - Add long-click event to trigger sharing - Reformat code style in HomePresenter for better readability
- Remove stringResource - Implement GraphSideEffect to handle one-time UI events like toasts
- Add missing trailing comma - Optimized imports
- Implement GraphDetailSideEffect to handle one-time UI events
📝 WalkthroughWalkthroughComprehensive refactoring introduces a side-effect pattern for UI notifications across multiple features, replacing state-based toast messages with sealed interface side-effects. Adds Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Feature UI
participant PresenterState as Presenter State
participant SideEffectHandler as SideEffect Handler<br/>(Composable)
participant EventHandler as EventHandler
participant Context as Android Context
UI->>PresenterState: User interaction triggers event<br/>(e.g., error, action)
PresenterState->>PresenterState: Update sideEffect field<br/>with sealed type<br/>(e.g., ShowToast)
PresenterState-->>UI: Emit new state with sideEffect
UI->>SideEffectHandler: Render SideEffect composable<br/>with new state
SideEffectHandler->>SideEffectHandler: LaunchedEffect observes<br/>state.sideEffect
alt ShowToast Side Effect
SideEffectHandler->>Context: Format message<br/>via asString(context)
Context-->>SideEffectHandler: Formatted string
SideEffectHandler->>EventHandler: Emit MetaSearchEvent.ShowToast
EventHandler-->>Context: Display toast
else ShareImage Side Effect
SideEffectHandler->>Context: Build Intent.ACTION_SEND<br/>with image URI
SideEffectHandler->>Context: Start activity with chooser
Context-->>Context: Share activity launched
end
SideEffectHandler->>UI: Emit InitSideEffect event<br/>via eventSink
UI->>PresenterState: Handle InitSideEffect
PresenterState->>PresenterState: Clear sideEffect to null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
core/ui/src/main/java/com/metasearch/android/core/ui/component/MetaSearchSquareImage.kt (1)
15-29:⚠️ Potential issue | 🟠 Major
combinedClickableunconditionally applied — restores unintended ripple & accessibility semantics when no handlers are set.The previous
clickableIfNotNullwas conditional: it applied the clickable modifier only whenonClickwas non-null. The newcombinedClickableis chained unconditionally, so even when bothonClickandonLongClickare null — the default for every non-interactive call site — the composable:
- Shows a ripple on tap (user sees feedback with no action).
- Gains "clickable" accessibility semantics (screen readers announce the element as interactive when it isn't).
Use the
enabledparameter to gate the modifier:🐛 Proposed fix using `enabled`
.combinedClickable( + enabled = onClick != null || onLongClick != null, onClick = { onClick?.invoke() }, onLongClick = { onLongClick?.invoke() }, ),Alternatively, skip the modifier entirely when neither callback is provided:
🐛 Proposed fix using conditional `then`
- .combinedClickable( - onClick = { onClick?.invoke() }, - onLongClick = { onLongClick?.invoke() }, - ), + .then( + if (onClick != null || onLongClick != null) { + Modifier.combinedClickable( + onClick = { onClick?.invoke() }, + onLongClick = { onLongClick?.invoke() }, + ) + } else Modifier + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/src/main/java/com/metasearch/android/core/ui/component/MetaSearchSquareImage.kt` around lines 15 - 29, MetaSearchSquareImage now always applies combinedClickable causing ripples and clickable semantics even when non-interactive; revert to conditional behavior by gating the modifier: when both onClick and onLongClick are null, do not apply combinedClickable (or set enabled = false) so AsyncImage does not get ripple/accessible clickable semantics. Locate the combinedClickable chained on the AsyncImage modifier and change it to apply only when onClick or onLongClick is non-null (or pass enabled = (onClick != null || onLongClick != null)); retain existing invocation of AsyncImage and null-safe invocation of onClick/onLongClick.feature/detail/src/main/java/com/metasearch/android/feature/detail/person/PersonDetailPresenter.kt (2)
137-144:⚠️ Potential issue | 🟠 Major
isLoadingis never reset ifsavePersonInfo()throws.There is no
try/finallyguard around the coroutine body. IfsavePersonInfo()(or any subsequent line) throws an uncaught exception the coroutine terminates withisLoading = true, permanently rendering the UI in a loading/blocked state.🛡️ Proposed fix
PersonDetailUiEvent.OnConfirmMergeSave -> { scope.launch { isLoading = true - savePersonInfo() - showMergeConfirmDialog = false - isLoading = false + try { + savePersonInfo() + showMergeConfirmDialog = false + } finally { + isLoading = false + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/detail/src/main/java/com/metasearch/android/feature/detail/person/PersonDetailPresenter.kt` around lines 137 - 144, The coroutine launched in the PersonDetailUiEvent.OnConfirmMergeSave handler sets isLoading = true but never guarantees it will be reset if savePersonInfo() throws; wrap the body of the scope.launch in a try/finally so that isLoading is set to false in the finally block (and keep showMergeConfirmDialog = false inside the try after successful save or set it appropriately on error), optionally catch/log exceptions from savePersonInfo() before rethrowing; update the lambda that references isLoading, savePersonInfo(), showMergeConfirmDialog and scope.launch to ensure isLoading is always cleared.
110-119:⚠️ Potential issue | 🟠 MajorTOCTOU:
personis re-read inside the coroutine instead of using the null-checkedcurrentPerson.
currentPersonis declared on line 111 solely as a null guard, butperson?.inputName(with a redundant safe-call) is used insidescope.launchinstead ofcurrentPerson.inputName. Becausepersonis acollectAsStatedelegate, its value is resolved at read time — ifpersonchanges tonullafter line 111 but before the coroutine body executes,person?.inputNamereturnsnull, makingeditName != nullvacuously true and spuriously triggering the merge confirm dialog.🔧 Proposed fix
is PersonDetailUiEvent.OnEditSaveClick -> { val currentPerson = person ?: return scope.launch { - if (editName != person?.inputName && personRepository.isNameExists(editName)) { + if (editName != currentPerson.inputName && personRepository.isNameExists(editName)) { showMergeConfirmDialog = true } else { savePersonInfo() } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/detail/src/main/java/com/metasearch/android/feature/detail/person/PersonDetailPresenter.kt` around lines 110 - 119, The TOCTOU bug: capture the null-checked snapshot `currentPerson` for use inside the coroutine instead of re-reading `person`; inside the `scope.launch` block replace `person?.inputName` with `currentPerson.inputName` (and use `currentPerson` wherever `person` would be read) so the comparison `editName != currentPerson.inputName` and the subsequent logic (`personRepository.isNameExists(editName)`, `showMergeConfirmDialog`, `savePersonInfo()`) operate on the stable, previously null-checked value.feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailPresenter.kt (1)
56-71:⚠️ Potential issue | 🟠 Major
MainScope()creates an unmanaged, leaked coroutine scope on every event.Each
OnPhotoSelectedevent creates a newMainScope()that is never cancelled, leaking coroutines if the composable leaves composition. UserememberCoroutineScope()at thepresent()level instead, consistent with howHomePresenterdoes it.Proposed fix
`@Composable` override fun present(): GraphDetailUiState { var sideEffect by remember { mutableStateOf<GraphDetailSideEffect?>(null) } var webViewUrl by remember { mutableStateOf("") } var selectedImages by remember { mutableStateOf(listOf<String>()) } val maxImages = 10 + val scope = rememberCoroutineScope() ... is GraphDetailUiEvent.OnPhotoSelected -> { - val scope = MainScope() scope.launch { val uri = graphRepository.findMatchedUri(event.photoName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailPresenter.kt` around lines 56 - 71, The handler for GraphDetailUiEvent.OnPhotoSelected is creating a new MainScope() per event which leaks; instead use a single coroutine scope provided to the presenter (like the scope created via rememberCoroutineScope() at present() as done in HomePresenter). Change the logic in the OnPhotoSelected branch to launch on that injected/present-level scope rather than calling MainScope(), keep using graphRepository.findMatchedUri(...) and update selectedImages and sideEffect the same way; ensure the presenter API (present()) accepts/uses a CoroutineScope (or stores a rememberCoroutineScope result) and that OnPhotoSelected uses that scope for scope.launch so coroutines are cancelled when the composable leaves composition.feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchPresenter.kt (2)
140-140:⚠️ Potential issue | 🟠 Major
TODO()will crash at runtime ifOnColorClickis dispatched.
TODO()throwsNotImplementedError. If there's a UI element wired to emit this event, the app will crash. Consider using a no-op or emitting a "not yet implemented" toast instead.Proposed fix
- FocusingSearchUiEvent.OnColorClick -> TODO() + FocusingSearchUiEvent.OnColorClick -> { + // TODO: Implement color click handling + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchPresenter.kt` at line 140, The when-branch handling FocusingSearchUiEvent.OnColorClick currently calls TODO(), which throws NotImplementedError at runtime; replace the TODO() in the FocusingSearchPresenter's event handler with a safe implementation — e.g. perform a no-op (ignore the event), dispatch a "not yet implemented" UI effect, or show a toast via the presenter's existing UI-effect mechanism — so that OnColorClick no longer crashes the app; update the branch inside the FocusingSearchPresenter (the when handling FocusingSearchUiEvent) to call the chosen safe action instead of TODO().
88-118:⚠️ Potential issue | 🟠 MajorTemp file may leak if the search coroutine is cancelled.
When
searchJob?.cancel()is called (line 88) or the coroutine is cancelled for other reasons,file.delete()on line 116 is skipped. Consider using atry/finallyblock to ensure cleanup.Proposed fix
searchJob = coroutineScope.launch { val uri = screen.imageUriString.toUri() val file = uri.toFile(context) - searchRepository.focusingSearch(file, circles) - .onSuccess { result -> - if (result.groups.isEmpty()) { - sideEffect = FocusingSearchSideEffect.ShowToast( - message = UiText.StringResource(R.string.search_screen_empty_result_message), - ) - } else { - searchResult = result - } - }.onFailure { exception -> - handleException( - exception = exception, - onError = { message -> - sideEffect = FocusingSearchSideEffect.ShowToast( - message = UiText.DynamicString(message), - ) - }, - ) - } - - file.delete() - isLoading = false + try { + searchRepository.focusingSearch(file, circles) + .onSuccess { result -> + if (result.groups.isEmpty()) { + sideEffect = FocusingSearchSideEffect.ShowToast( + message = UiText.StringResource(R.string.search_screen_empty_result_message), + ) + } else { + searchResult = result + } + }.onFailure { exception -> + handleException( + exception = exception, + onError = { message -> + sideEffect = FocusingSearchSideEffect.ShowToast( + message = UiText.DynamicString(message), + ) + }, + ) + } + } finally { + file.delete() + isLoading = false + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchPresenter.kt` around lines 88 - 118, The temp file deletion and isLoading reset can be skipped when the coroutine launched by coroutineScope.launch (assigned to searchJob) is cancelled; wrap the call to searchRepository.focusingSearch(...) and its onSuccess/onFailure handling in a try/finally inside the coroutine so that file.delete() and isLoading = false always run in the finally block (check file.exists() before deleting and swallow/log any deletion errors), ensuring searchJob cancellation cannot leak the temp file or leave isLoading true.
🧹 Nitpick comments (9)
core/common/src/main/java/com/metasearch/android/core/common/utils/UiText.kt (1)
12-15:StringResourceis intentionally a plainclass— worth a brief comment to avoid future "fix" PRs.Using
data classhere would be incorrect because Kotlin arrays (vararg val args: Any→Array<out Any>) don't provide structural equality, so the generatedequals/hashCodewould break. The currentclasschoice is correct, but a one-line comment stating this prevents well-intentioned refactors that would introduce a subtle bug.📝 Suggested documentation
+ // Not a data class: vararg args is stored as Array, which lacks structural equality. class StringResource( `@StringRes` val resId: Int, vararg val args: Any, ) : UiText🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/common/src/main/java/com/metasearch/android/core/common/utils/UiText.kt` around lines 12 - 15, Add a one-line comment above the StringResource class explaining why it is a plain class (not a data class): state that args is a vararg Array<out Any> so using a data class would use reference equality for the args array and break generated equals/hashCode, therefore StringResource implements UiText as a regular class intentionally to avoid subtle equality bugs.feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailUiState.kt (1)
16-22:@Immutablecontract is not fully satisfied becauseUiText.StringResource.argsis a mutableArray.
@Immutablepromises Compose that all public properties are deeply stable.UiTextis not annotated@Stable/@Immutable, andUiText.StringResourcestoresvararg val args: Anyas anArray<out Any>(mutable). This is a soft violation — no runtime crash, and in practiceargsis never mutated — but Compose may make incorrect skipping decisions based on the declared contract. Consider either removing@Immutableand relying on the UUID key for change detection, or annotatingUiTextappropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailUiState.kt` around lines 16 - 22, The GraphDetailSideEffect sealed type is annotated `@Immutable` but exposes a UiText (UiText.StringResource) which contains a mutable Array for vararg args, violating the `@Immutable` contract; update the implementation by either removing the `@Immutable` annotation from GraphDetailSideEffect, or change ShowToast to expose an immutable representation (e.g., a plain String message or a stable wrapper) instead of UiText, or annotate/modify UiText/StringResource to be `@Stable/`@Immutable and ensure its args are stored in an immutable List; target the GraphDetailSideEffect/ShowToast types and the UiText/StringResource usage and pick one of these fixes so Compose’s immutability contract is satisfied.feature/person/src/main/java/com/metasearch/android/feature/person/PersonSideEffect.kt (2)
10-10: Naming collision between composable function and sealed interface.The composable function
PersonSideEffectshares its name with thePersonSideEffectsealed interface. While Kotlin allows a top-level function and a type to coexist with the same name, it creates readability confusion — e.g., on Line 18PersonSideEffect.ShowToastrefers to the interface, not the enclosing function. Consider renaming the composable to something likeHandlePersonSideEffectorPersonSideEffectHandler.The same issue exists across all other side-effect handler composables in this PR (
FocusingSearchSideEffect,GraphSideEffect,NLSearchSideEffect, etc.).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/person/src/main/java/com/metasearch/android/feature/person/PersonSideEffect.kt` at line 10, The top-level composable function PersonSideEffect conflicts in name with the sealed interface PersonSideEffect, causing readability confusion; rename the composable (e.g., to HandlePersonSideEffect or PersonSideEffectHandler) and update all call sites and imports accordingly so usages like PersonSideEffect.ShowToast still refer to the sealed interface; apply the same rename pattern for other side-effect handler composables in this PR (FocusingSearchSideEffect, GraphSideEffect, NLSearchSideEffect, etc.) to avoid name collisions and improve clarity.
1-33: Heavy code duplication across all side-effect handlers.This composable is virtually identical to
FocusingSearchSideEffect,GraphSideEffect,NLSearchSideEffect, etc. — the only differences are the type names. Consider extracting a generic/reusable handler, e.g.:`@Composable` fun <S, E> HandleSideEffect( sideEffect: S?, onEffect: suspend (S) -> Unit, onReset: () -> Unit, ) { LaunchedEffect(sideEffect) { if (sideEffect != null) { onEffect(sideEffect) onReset() } } }Each feature would then only need to supply its specific mapping from side-effect to action.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/person/src/main/java/com/metasearch/android/feature/person/PersonSideEffect.kt` around lines 1 - 33, Extract a generic composable to handle nullable side-effect objects and reuse it from PersonSideEffect: create e.g. HandleSideEffect<S> that takes sideEffect: S?, onEffect: suspend (S) -> Unit, and onReset: () -> Unit, move the LaunchedEffect logic there and call EventHandler.sendEvent(MetaSearchEvent.ShowToast(...)) from the onEffect lambda for Person side-effects; then in PersonSideEffect replace the LaunchedEffect block by calling HandleSideEffect(state.sideEffect, { effect -> EventHandler.sendEvent(MetaSearchEvent.ShowToast(message = effect.message.asString(LocalContext.current))) }, { eventSink(PersonUiEvent.InitSideEffect) }) so the unique symbols PersonSideEffect, PersonUiState, PersonUiEvent.InitSideEffect, EventHandler and MetaSearchEvent.ShowToast are retained and duplication removed.feature/home/src/main/java/com/metasearch/android/feature/home/HomePresenter.kt (1)
124-131: Nit:offSetshould beoffset(standard Kotlin/Compose naming).The event field
event.offSetuses inconsistent casing — Compose's own type isOffset, and the convention for property names would beoffset. This originates from theHomeUiEvent.OnImageLongClickdefinition. Consider fixing it at the source.Also,
OnLongClickCancelclearsselectedLongClickImagebut leavesselectedOffsetstale. While harmless (offset is only relevant when the image is non-null), resetting it would be cleaner.Proposed fix
HomeUiEvent.OnLongClickCancel -> { selectedLongClickImage = null + selectedOffset = Offset.Zero }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomePresenter.kt` around lines 124 - 131, Rename the event property offSet to offset in the HomeUiEvent.OnImageLongClick definition and update all usages (e.g., in HomePresenter where you access event.offSet) to event.offset to match Kotlin/Compose naming; additionally, when handling HomeUiEvent.OnLongClickCancel in HomePresenter (where you currently set selectedLongClickImage = null), also clear selectedOffset (set it to null) so the offset doesn't remain stale when no image is selected.feature/graph/src/main/java/com/metasearch/android/feature/graph/GraphUi.kt (1)
96-104: Pre-existing: SSL errors are silently accepted.This is not part of the current change, but
handler?.proceed()unconditionally bypasses all SSL certificate errors, which is a significant security risk (allows MitM attacks). Consider addressing this in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/graph/src/main/java/com/metasearch/android/feature/graph/GraphUi.kt` around lines 96 - 104, The WebViewClient currently unconditionally bypasses SSL errors in onReceivedSslError by calling handler?.proceed(), which is a security risk; update onReceivedSslError (in the anonymous WebViewClient) to not silently proceed: either call handler?.cancel() and surface a user-facing warning/dialog plus logging, or restrict handler?.proceed() to non-production builds only (e.g., guard with BuildConfig.DEBUG) and otherwise cancel; ensure the change touches the onReceivedSslError override and removes the unconditional handler?.proceed() behavior.feature/home/src/main/java/com/metasearch/android/feature/home/HomeUi.kt (1)
305-348: Remove commented-out preview code.The large
PersonModelblock (lines 305–348) is dead code left over from an earlier state. It should be cleaned up to reduce noise in the preview.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUi.kt` around lines 305 - 348, Remove the commented-out preview data block that defines sample PersonModel and FaceModel instances in HomeUi (the large commented list of PersonModel/FaceModel entries); delete the entire commented section starting with the persons = list(...) and all nested PersonModel/FaceModel entries to clean up dead preview code and leave only active preview/state setup.feature/home/src/main/java/com/metasearch/android/feature/home/HomeSideEffect.kt (1)
10-11: Naming collision between composable function and sealed interface.
fun HomeSideEffectandsealed interface HomeSideEffectshare the same name in the same package (com.metasearch.android.feature.home). While valid Kotlin (callables and types occupy different namespaces), this can be confusing when reading call sites or performing IDE lookups — e.g.,HomeSideEffect(...)is ambiguous at a glance. The same collision exists across all other*SideEffectfiles in this PR.Consider a conventional suffix to disambiguate — e.g.,
HomeSideEffectHandlerfor the composable, keeping the sealed interface asHomeSideEffect. This is consistent with how other Compose handler composables are commonly named.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeSideEffect.kt` around lines 10 - 11, The composable function named HomeSideEffect conflicts with the sealed interface HomeSideEffect; rename the composable (e.g., change fun HomeSideEffect(...) to HomeSideEffectHandler(...)) and update all call sites/imports to use the new name while leaving the sealed interface HomeSideEffect unchanged so types and handlers are clearly disambiguated; apply the same rename pattern to all other *SideEffect composables in this PR.feature/home/src/main/java/com/metasearch/android/feature/home/HomeUiState.kt (1)
59-62: Naming:offSetshould beoffset.The parameter
offSetuses an unconventional camelCase split. Kotlin convention (and the ComposeOffsetclass name itself) treat "offset" as a single word.Proposed fix
data class OnImageLongClick( val imageUriString: String, - val offSet: Offset = Offset.Zero, + val offset: Offset = Offset.Zero, ) : HomeUiEvent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUiState.kt` around lines 59 - 62, Rename the parameter offSet to offset in the OnImageLongClick data class to follow Kotlin naming conventions; update the constructor parameter name (val offSet: Offset = Offset.Zero -> val offset: Offset = Offset.Zero) and then update all call sites and usages of OnImageLongClick.offset (including any pattern matches, destructuring, or references) to use the new name so compilation and semantics remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build-logic/src/main/java/AndroidFeatureConventionPlugin.kt`:
- Line 26: The build uses implementation(libs.kotlinx.collections.immutable) but
FocusingSearchUiState exposes ImmutableList<CircleModel> publicly, so change the
dependency declaration to api(libs.kotlinx.collections.immutable) in the
AndroidFeatureConventionPlugin (the place where kotlinx-collections-immutable is
added) so the ImmutableList type is transitive and available on consumers'
compile classpath; update the dependency configuration from implementation to
api for kotlinx-collections-immutable to fix the missing type at compile time.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailSideEffect.kt`:
- Around line 17-19: The GraphDetailSideEffect.ShowToast branch builds a
MetaSearchEvent.ShowToast but never dispatches it and lacks the EventHandler
import; update the branch in GraphDetailSideEffect (where
GraphDetailSideEffect.ShowToast is handled) to call
EventHandler.sendEvent(MetaSearchEvent.ShowToast(...)) instead of just
constructing the event, and add the missing import for EventHandler so the
sendEvent call compiles; follow the same pattern used in NLSearchSideEffect
(lines handling EventHandler.sendEvent) as a template.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/photo/PhotoDetailPresenter.kt`:
- Around line 97-101: Add a UUID-based key to the ShareImage side effect so
consecutive shares with the same uriString are not considered equal: modify the
PhotoDetailSideEffect.ShareImage data class to include a private val key: String
= UUID.randomUUID().toString() (and add import java.util.UUID) so existing
construction in PhotoDetailPresenter (handling
PhotoDetailUiEvent.OnShareImageButtonClick) can remain unchanged but each
instance is unique; mirror the pattern used by ShowToast for deduplication.
In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUi.kt`:
- Around line 125-130: The offset lambda is using hardcoded pixel constants
(+20, -120) which breaks across screen densities; change those raw numbers to
dp-to-px conversions using the Density receiver available in the offset { }
lambda (e.g., call 20.dp.toPx() and 120.dp.toPx()) before converting to Int for
IntOffset so the offsets are consistent with state.selectedOffset (which comes
from coordinates.positionInRoot()) and then use (x = (state.selectedOffset.x +
convertedX).toInt(), y = (state.selectedOffset.y - convertedY).toInt()) inside
the offset block.
- Around line 118-155: The overlay content is removed immediately because
state.selectedLongClickImage is set to null before AnimatedVisibility's fadeOut
finishes; fix by introducing a local "last non-null" snapshot (e.g.,
lastSelectedLongClickImage) that you update when state.selectedLongClickImage
becomes non-null and continue to use that snapshot instead of
state.selectedLongClickImage inside the let block that renders Icon and
AsyncImage; clear the snapshot only after the exit animation completes (use
AnimatedVisibility's exit/onAnimationEnd callback or a LaunchedEffect keyed to
the visibility/animation) so the Icon and AsyncImage remain visible for the full
fadeOut when OnLongClickCancel fires.
In
`@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUiState.kt`:
- Around line 25-30: HomeSideEffect.ShareImage currently lacks a uniqueness key
so emitting the same uriString twice produces an equal object and Compose won't
re-trigger the side effect; update the ShareImage data class inside
HomeSideEffect to include a private val key: String =
UUID.randomUUID().toString() (import java.util.UUID as needed) so each emission
is distinct while keeping uriString accessible.
In `@feature/home/src/main/res/drawable/ic_image_share.xml`:
- Around line 2-3: The drawable's intrinsic size is incorrectly large: replace
the android:width and android:height attributes set to "512dp" with the correct
icon size "24dp" so the vector's intrinsic dimensions match its viewport
(viewportWidth="24" and viewportHeight="24"); update the attributes in
ic_image_share.xml to android:width="24dp" and android:height="24dp" to prevent
the drawable from occupying 512dp when used with wrap_content or default
ImageView behavior.
In
`@feature/main/src/main/java/com/metasearch/android/feature/main/MainActivity.kt`:
- Around line 106-109: The toast currently passes a non-null empty string via
MetaSearchToast(message = toastMessage ?: ""), causing the message?.let { ... }
block in MetaSearchToast to run during exit animation; change the call to pass
the nullable value directly (message = toastMessage) so MetaSearchToast.message
(String? = null) can be null and the let block will be skipped when there is no
message.
In
`@feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.kt`:
- Around line 53-55: Replace the three delete-dialog state properties that
currently use remember with rememberRetained so the confirmation dialog and
pending values persist across configuration changes: update showDeleteDialog,
pendingDeletePersonId and pendingDeletePersonName to use rememberRetained
instead of remember (matching the existing usage for the search input and other
dialog state in PersonDetailPresenter) to prevent the dialog from dismissing and
the id/name from becoming stale on rotation.
In
`@feature/person/src/main/java/com/metasearch/android/feature/person/PersonSideEffect.kt`:
- Around line 16-32: The when on state.sideEffect in LaunchedEffect uses an else
branch which hides unhandled new sealed subtypes; change the catch-all from else
-> {} to null -> {} so the Kotlin compiler can enforce exhaustiveness for the
sealed interface (update the when handling around
LaunchedEffect(state.sideEffect), keeping the PersonSideEffect.ShowToast branch
and the subsequent eventSink(PersonUiEvent.InitSideEffect) logic intact). Ensure
any other similar handlers (e.g., in Graph/FocusingSearch) follow the same null
-> {} pattern.
---
Outside diff comments:
In
`@core/ui/src/main/java/com/metasearch/android/core/ui/component/MetaSearchSquareImage.kt`:
- Around line 15-29: MetaSearchSquareImage now always applies combinedClickable
causing ripples and clickable semantics even when non-interactive; revert to
conditional behavior by gating the modifier: when both onClick and onLongClick
are null, do not apply combinedClickable (or set enabled = false) so AsyncImage
does not get ripple/accessible clickable semantics. Locate the combinedClickable
chained on the AsyncImage modifier and change it to apply only when onClick or
onLongClick is non-null (or pass enabled = (onClick != null || onLongClick !=
null)); retain existing invocation of AsyncImage and null-safe invocation of
onClick/onLongClick.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailPresenter.kt`:
- Around line 56-71: The handler for GraphDetailUiEvent.OnPhotoSelected is
creating a new MainScope() per event which leaks; instead use a single coroutine
scope provided to the presenter (like the scope created via
rememberCoroutineScope() at present() as done in HomePresenter). Change the
logic in the OnPhotoSelected branch to launch on that injected/present-level
scope rather than calling MainScope(), keep using
graphRepository.findMatchedUri(...) and update selectedImages and sideEffect the
same way; ensure the presenter API (present()) accepts/uses a CoroutineScope (or
stores a rememberCoroutineScope result) and that OnPhotoSelected uses that scope
for scope.launch so coroutines are cancelled when the composable leaves
composition.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/person/PersonDetailPresenter.kt`:
- Around line 137-144: The coroutine launched in the
PersonDetailUiEvent.OnConfirmMergeSave handler sets isLoading = true but never
guarantees it will be reset if savePersonInfo() throws; wrap the body of the
scope.launch in a try/finally so that isLoading is set to false in the finally
block (and keep showMergeConfirmDialog = false inside the try after successful
save or set it appropriately on error), optionally catch/log exceptions from
savePersonInfo() before rethrowing; update the lambda that references isLoading,
savePersonInfo(), showMergeConfirmDialog and scope.launch to ensure isLoading is
always cleared.
- Around line 110-119: The TOCTOU bug: capture the null-checked snapshot
`currentPerson` for use inside the coroutine instead of re-reading `person`;
inside the `scope.launch` block replace `person?.inputName` with
`currentPerson.inputName` (and use `currentPerson` wherever `person` would be
read) so the comparison `editName != currentPerson.inputName` and the subsequent
logic (`personRepository.isNameExists(editName)`, `showMergeConfirmDialog`,
`savePersonInfo()`) operate on the stable, previously null-checked value.
In
`@feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchPresenter.kt`:
- Line 140: The when-branch handling FocusingSearchUiEvent.OnColorClick
currently calls TODO(), which throws NotImplementedError at runtime; replace the
TODO() in the FocusingSearchPresenter's event handler with a safe implementation
— e.g. perform a no-op (ignore the event), dispatch a "not yet implemented" UI
effect, or show a toast via the presenter's existing UI-effect mechanism — so
that OnColorClick no longer crashes the app; update the branch inside the
FocusingSearchPresenter (the when handling FocusingSearchUiEvent) to call the
chosen safe action instead of TODO().
- Around line 88-118: The temp file deletion and isLoading reset can be skipped
when the coroutine launched by coroutineScope.launch (assigned to searchJob) is
cancelled; wrap the call to searchRepository.focusingSearch(...) and its
onSuccess/onFailure handling in a try/finally inside the coroutine so that
file.delete() and isLoading = false always run in the finally block (check
file.exists() before deleting and swallow/log any deletion errors), ensuring
searchJob cancellation cannot leak the temp file or leave isLoading true.
---
Duplicate comments:
In
`@feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchSideEffect.kt`:
- Around line 9-32: The composable FocusingSearchSideEffect collides with the
sealed interface of the same name and uses a broad else branch in the when;
rename the composable (e.g., to FocusingSearchSideEffectHandler or
FocusingSearchSideEffectEffect) everywhere it’s used to avoid the type/name
collision, update its function declaration accordingly, and change the when
branch from else -> {} to null -> {} so the when explicitly handles the
absent-case; also update any imports/usages of the sealed interface and the
composable to the new names to keep references consistent and preserve the
existing eventSink(FocusingSearchUiEvent.InitSideEffect) behavior when
state.sideEffect != null.
---
Nitpick comments:
In
`@core/common/src/main/java/com/metasearch/android/core/common/utils/UiText.kt`:
- Around line 12-15: Add a one-line comment above the StringResource class
explaining why it is a plain class (not a data class): state that args is a
vararg Array<out Any> so using a data class would use reference equality for the
args array and break generated equals/hashCode, therefore StringResource
implements UiText as a regular class intentionally to avoid subtle equality
bugs.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailUiState.kt`:
- Around line 16-22: The GraphDetailSideEffect sealed type is annotated
`@Immutable` but exposes a UiText (UiText.StringResource) which contains a mutable
Array for vararg args, violating the `@Immutable` contract; update the
implementation by either removing the `@Immutable` annotation from
GraphDetailSideEffect, or change ShowToast to expose an immutable representation
(e.g., a plain String message or a stable wrapper) instead of UiText, or
annotate/modify UiText/StringResource to be `@Stable/`@Immutable and ensure its
args are stored in an immutable List; target the GraphDetailSideEffect/ShowToast
types and the UiText/StringResource usage and pick one of these fixes so
Compose’s immutability contract is satisfied.
In `@feature/graph/src/main/java/com/metasearch/android/feature/graph/GraphUi.kt`:
- Around line 96-104: The WebViewClient currently unconditionally bypasses SSL
errors in onReceivedSslError by calling handler?.proceed(), which is a security
risk; update onReceivedSslError (in the anonymous WebViewClient) to not silently
proceed: either call handler?.cancel() and surface a user-facing warning/dialog
plus logging, or restrict handler?.proceed() to non-production builds only
(e.g., guard with BuildConfig.DEBUG) and otherwise cancel; ensure the change
touches the onReceivedSslError override and removes the unconditional
handler?.proceed() behavior.
In
`@feature/home/src/main/java/com/metasearch/android/feature/home/HomePresenter.kt`:
- Around line 124-131: Rename the event property offSet to offset in the
HomeUiEvent.OnImageLongClick definition and update all usages (e.g., in
HomePresenter where you access event.offSet) to event.offset to match
Kotlin/Compose naming; additionally, when handling HomeUiEvent.OnLongClickCancel
in HomePresenter (where you currently set selectedLongClickImage = null), also
clear selectedOffset (set it to null) so the offset doesn't remain stale when no
image is selected.
In
`@feature/home/src/main/java/com/metasearch/android/feature/home/HomeSideEffect.kt`:
- Around line 10-11: The composable function named HomeSideEffect conflicts with
the sealed interface HomeSideEffect; rename the composable (e.g., change fun
HomeSideEffect(...) to HomeSideEffectHandler(...)) and update all call
sites/imports to use the new name while leaving the sealed interface
HomeSideEffect unchanged so types and handlers are clearly disambiguated; apply
the same rename pattern to all other *SideEffect composables in this PR.
In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUi.kt`:
- Around line 305-348: Remove the commented-out preview data block that defines
sample PersonModel and FaceModel instances in HomeUi (the large commented list
of PersonModel/FaceModel entries); delete the entire commented section starting
with the persons = list(...) and all nested PersonModel/FaceModel entries to
clean up dead preview code and leave only active preview/state setup.
In
`@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUiState.kt`:
- Around line 59-62: Rename the parameter offSet to offset in the
OnImageLongClick data class to follow Kotlin naming conventions; update the
constructor parameter name (val offSet: Offset = Offset.Zero -> val offset:
Offset = Offset.Zero) and then update all call sites and usages of
OnImageLongClick.offset (including any pattern matches, destructuring, or
references) to use the new name so compilation and semantics remain correct.
In
`@feature/person/src/main/java/com/metasearch/android/feature/person/PersonSideEffect.kt`:
- Line 10: The top-level composable function PersonSideEffect conflicts in name
with the sealed interface PersonSideEffect, causing readability confusion;
rename the composable (e.g., to HandlePersonSideEffect or
PersonSideEffectHandler) and update all call sites and imports accordingly so
usages like PersonSideEffect.ShowToast still refer to the sealed interface;
apply the same rename pattern for other side-effect handler composables in this
PR (FocusingSearchSideEffect, GraphSideEffect, NLSearchSideEffect, etc.) to
avoid name collisions and improve clarity.
- Around line 1-33: Extract a generic composable to handle nullable side-effect
objects and reuse it from PersonSideEffect: create e.g. HandleSideEffect<S> that
takes sideEffect: S?, onEffect: suspend (S) -> Unit, and onReset: () -> Unit,
move the LaunchedEffect logic there and call
EventHandler.sendEvent(MetaSearchEvent.ShowToast(...)) from the onEffect lambda
for Person side-effects; then in PersonSideEffect replace the LaunchedEffect
block by calling HandleSideEffect(state.sideEffect, { effect ->
EventHandler.sendEvent(MetaSearchEvent.ShowToast(message =
effect.message.asString(LocalContext.current))) }, {
eventSink(PersonUiEvent.InitSideEffect) }) so the unique symbols
PersonSideEffect, PersonUiState, PersonUiEvent.InitSideEffect, EventHandler and
MetaSearchEvent.ShowToast are retained and duplication removed.
| implementationProject(":core:model") | ||
| implementationProject(":feature:screens") | ||
|
|
||
| implementation(libs.kotlinx.collections.immutable) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find modules that depend on feature modules but are not feature modules themselves
echo "=== Modules applying AndroidFeatureConventionPlugin ==="
rg -l 'metasearch.android.feature' --include="*.gradle.kts" --include="*.gradle"
echo ""
echo "=== Modules depending on feature modules ==="
rg 'implementation.*:feature:' --include="*.gradle.kts" --include="*.gradle" -A1Repository: komodgn/meta-android
Length of output: 1066
🏁 Script executed:
# Search for State classes with ImmutableList in feature modules
echo "=== State classes in feature modules ==="
find . -path "*/feature/*/src/*" -name "*State.kt" -type f | head -20
echo ""
echo "=== ImmutableList usage in feature modules ==="
rg 'ImmutableList' --include="*.kt" -l | grep featureRepository: komodgn/meta-android
Length of output: 1523
🏁 Script executed:
# Check the current dependency scope in the convention plugin
echo "=== AndroidFeatureConventionPlugin content ==="
cat build-logic/src/main/java/AndroidFeatureConventionPlugin.kt | head -60Repository: komodgn/meta-android
Length of output: 1392
🏁 Script executed:
#!/bin/bash
# Check if State classes actually use ImmutableList
echo "=== Checking State classes for ImmutableList ==="
for file in ./feature/*/src/main/java/com/metasearch/android/feature/*/\*UiState.kt; do
if [ -f "$file" ]; then
echo "File: $file"
head -30 "$file"
echo "---"
fi
done | head -150Repository: komodgn/meta-android
Length of output: 112
🏁 Script executed:
# Search for modules depending on feature modules using correct rg syntax
echo "=== Gradle files with feature dependencies ==="
rg ':feature:' -t gradle -t kotlin | head -30
echo ""
echo "=== App module build file ==="
if [ -f "app/build.gradle.kts" ]; then
cat app/build.gradle.kts | grep -E 'feature|implementation|dependencies' -A2 -B2 | head -50
fiRepository: komodgn/meta-android
Length of output: 1701
🏁 Script executed:
# Check what modules apply AndroidFeatureConventionPlugin
echo "=== Feature modules with convention plugin ==="
find . -path "*/feature/*/build.gradle.kts" -exec grep -l "metasearch.android.feature" {} \;Repository: komodgn/meta-android
Length of output: 413
🏁 Script executed:
#!/bin/bash
# Check individual State files for ImmutableList
echo "=== HomeUiState ==="
cat ./feature/home/src/main/java/com/metasearch/android/feature/home/HomeUiState.kt
echo ""
echo "=== GraphUiState ==="
cat ./feature/graph/src/main/java/com/metasearch/android/feature/graph/GraphUiState.kt
echo ""
echo "=== NLSearchUiState ==="
cat ./feature/search/src/main/java/com/metasearch/android/feature/search/nls/NLSearchUiState.ktRepository: komodgn/meta-android
Length of output: 4754
🏁 Script executed:
# Search for ImmutableList usage across the codebase
rg 'ImmutableList' -A2 -B2 | head -80Repository: komodgn/meta-android
Length of output: 1540
🏁 Script executed:
# Check if any non-feature modules use State classes from feature modules
echo "=== Non-feature modules that depend on feature modules ==="
rg 'feature\.' --type gradle --type kotlin | grep -E 'implementation|api' | grep -v feature/ | head -20Repository: komodgn/meta-android
Length of output: 704
🏁 Script executed:
# Search for all State classes with ImmutableList to get a complete picture
rg 'ImmutableList' -B5 | grep -E 'package|ImmutableList' | head -40
# Also check core:model to see if any model classes use ImmutableList
echo ""
echo "=== ImmutableList in model classes ==="
rg 'ImmutableList' core/model/ -A1 -B1 | head -40Repository: komodgn/meta-android
Length of output: 424
🏁 Script executed:
# Check if the app module would have compilation issues
# by verifying if kotlinx-collections-immutable is in app module's effective dependencies
echo "=== App module build.gradle.kts dependencies section ==="
sed -n '/dependencies {/,/^}/p' app/build.gradle.kts | grep -i immutableRepository: komodgn/meta-android
Length of output: 120
🏁 Script executed:
# Verify only FocusingSearchUiState uses ImmutableList
echo "=== Complete ImmutableList usage across feature modules ==="
rg 'ImmutableList' feature/ --include="*.kt" -B2
# Check if app module would compile with current setup
echo ""
echo "=== Gradle dependency resolution check ==="
echo "App module dependencies on feature modules (all implementation scope):"
grep 'feature\.' app/build.gradle.kts | grep implementation | wc -l
echo "Number of feature modules with implementation(libs.kotlinx.collections.immutable):"
echo "9 (all feature modules via convention plugin)"Repository: komodgn/meta-android
Length of output: 828
🏁 Script executed:
# Check what other core modules might expose ImmutableList
rg 'ImmutableList' core/ --include="*.kt" | head -20Repository: komodgn/meta-android
Length of output: 505
🏁 Script executed:
# Check if app module or other modules import State classes from features
rg 'FocusingSearchUiState|HomeUiState|GraphUiState|NLSearchUiState' --type kotlin | grep -v 'feature.*src.*' | head -20
echo ""
echo "=== Check FocusingSearchUiState file directly ==="
cat feature/search/src/main/java/com/metasearch/android/feature/search/focusing/FocusingSearchUiState.ktRepository: komodgn/meta-android
Length of output: 2134
🏁 Script executed:
# Verify if ImmutableList appears in public API of any State classes
rg 'ImmutableList' -B10 -A5 feature/ --type kotlin | grep -E 'package|class|data class|val.*:.*ImmutableList'Repository: komodgn/meta-android
Length of output: 523
Use api scope for kotlinx-collections-immutable to ensure transitive availability for State types.
FocusingSearchUiState exposes ImmutableList<CircleModel> as a public field—a type that becomes part of the public API. With implementation scope, the dependency is not transitive; modules depending on feature modules won't have ImmutableList on their compile classpath. While most State types are consumed through the Circuit runtime and may not require direct type references, any code that imports these State classes (for testing, navigation setup, or composition) will fail to compile without the dependency.
Changing to api scope ensures the type is available to all consumers of feature modules and eliminates a potential compilation surprise.
Suggested change
- implementation(libs.kotlinx.collections.immutable)
+ api(libs.kotlinx.collections.immutable)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| implementation(libs.kotlinx.collections.immutable) | |
| api(libs.kotlinx.collections.immutable) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build-logic/src/main/java/AndroidFeatureConventionPlugin.kt` at line 26, The
build uses implementation(libs.kotlinx.collections.immutable) but
FocusingSearchUiState exposes ImmutableList<CircleModel> publicly, so change the
dependency declaration to api(libs.kotlinx.collections.immutable) in the
AndroidFeatureConventionPlugin (the place where kotlinx-collections-immutable is
added) so the ImmutableList type is transitive and available on consumers'
compile classpath; update the dependency configuration from implementation to
api for kotlinx-collections-immutable to fix the missing type at compile time.
| is GraphDetailSideEffect.ShowToast -> { | ||
| MetaSearchEvent.ShowToast(message = state.sideEffect.message.asString(context)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all SideEffect-related files and locate the files mentioned in the review
find . -name "*SideEffect*" -type f | head -20Repository: komodgn/meta-android
Length of output: 818
🏁 Script executed:
# Check the current state of GraphDetailSideEffect.kt, specifically around lines 17-19
find . -name "GraphDetailSideEffect.kt" -type fRepository: komodgn/meta-android
Length of output: 163
🏁 Script executed:
# Also search for NLSearchSideEffect.kt
find . -name "NLSearchSideEffect.kt" -type fRepository: komodgn/meta-android
Length of output: 158
🏁 Script executed:
cat -n ./feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailSideEffect.ktRepository: komodgn/meta-android
Length of output: 1087
🏁 Script executed:
cat -n ./feature/search/src/main/java/com/metasearch/android/feature/search/nls/NLSearchSideEffect.ktRepository: komodgn/meta-android
Length of output: 1281
🏁 Script executed:
# Search for EventHandler usage pattern to understand the correct dispatch method
rg "EventHandler\.sendEvent" --type kotlin -B 2 -A 2 | head -50Repository: komodgn/meta-android
Length of output: 4050
🏁 Script executed:
# Check if there are any other ShowToast or EventHandler references in GraphDetailSideEffect.kt
rg "ShowToast|EventHandler" ./feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailSideEffect.ktRepository: komodgn/meta-android
Length of output: 212
🏁 Script executed:
# Verify the line count of GraphDetailSideEffect.kt to ensure we're seeing the full file
wc -l ./feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailSideEffect.ktRepository: komodgn/meta-android
Length of output: 166
Add missing EventHandler import and dispatch ShowToast event
MetaSearchEvent.ShowToast is constructed but never dispatched — toast will silently not appear. The event object must be wrapped with EventHandler.sendEvent(), consistent with the correct pattern in NLSearchSideEffect.kt (lines 19-23). The EventHandler import is also missing.
Fix
+import com.metasearch.android.core.common.utils.EventHandler
import com.metasearch.android.core.common.utils.MetaSearchEvent is GraphDetailSideEffect.ShowToast -> {
- MetaSearchEvent.ShowToast(message = state.sideEffect.message.asString(context))
+ EventHandler.sendEvent(
+ MetaSearchEvent.ShowToast(message = state.sideEffect.message.asString(context))
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| is GraphDetailSideEffect.ShowToast -> { | |
| MetaSearchEvent.ShowToast(message = state.sideEffect.message.asString(context)) | |
| } | |
| import com.metasearch.android.core.common.utils.EventHandler | |
| import com.metasearch.android.core.common.utils.MetaSearchEvent |
| is GraphDetailSideEffect.ShowToast -> { | |
| MetaSearchEvent.ShowToast(message = state.sideEffect.message.asString(context)) | |
| } | |
| is GraphDetailSideEffect.ShowToast -> { | |
| EventHandler.sendEvent( | |
| MetaSearchEvent.ShowToast(message = state.sideEffect.message.asString(context)) | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/graph/GraphDetailSideEffect.kt`
around lines 17 - 19, The GraphDetailSideEffect.ShowToast branch builds a
MetaSearchEvent.ShowToast but never dispatches it and lacks the EventHandler
import; update the branch in GraphDetailSideEffect (where
GraphDetailSideEffect.ShowToast is handled) to call
EventHandler.sendEvent(MetaSearchEvent.ShowToast(...)) instead of just
constructing the event, and add the missing import for EventHandler so the
sendEvent call compiles; follow the same pattern used in NLSearchSideEffect
(lines handling EventHandler.sendEvent) as a template.
| is PhotoDetailUiEvent.OnShareImageButtonClick -> { | ||
| sideEffect = PhotoDetailSideEffect.ShareImage( | ||
| uriString = event.imageUriString, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "PhotoDetailUiState.kt" --exec cat {}Repository: komodgn/meta-android
Length of output: 1680
Add a UUID key to PhotoDetailSideEffect.ShareImage for proper deduplication.
The ShareImage side effect only contains uriString and lacks the UUID key present in ShowToast. Sharing the same image URI consecutively will produce structurally equal side effects, blocking the second emission. Add private val key: String = UUID.randomUUID().toString() to ensure each ShareImage instance is unique.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/detail/src/main/java/com/metasearch/android/feature/detail/photo/PhotoDetailPresenter.kt`
around lines 97 - 101, Add a UUID-based key to the ShareImage side effect so
consecutive shares with the same uriString are not considered equal: modify the
PhotoDetailSideEffect.ShareImage data class to include a private val key: String
= UUID.randomUUID().toString() (and add import java.util.UUID) so existing
construction in PhotoDetailPresenter (handling
PhotoDetailUiEvent.OnShareImageButtonClick) can remain unchanged but each
instance is unique; mirror the pattern used by ShowToast for deduplication.
| state.selectedLongClickImage?.let { uri -> | ||
| Icon( | ||
| painter = painterResource(R.drawable.ic_image_share), | ||
| contentDescription = null, | ||
| tint = LightPink, | ||
| modifier = Modifier | ||
| .align(Alignment.TopStart) | ||
| .offset { | ||
| IntOffset( | ||
| x = (state.selectedOffset.x + 20).toInt(), | ||
| y = (state.selectedOffset.y - 120).toInt(), | ||
| ) | ||
| } | ||
| .size(50.dp) | ||
| .clickable { | ||
| state.eventSink(HomeUiEvent.OnShareRelease(uri)) | ||
| }, | ||
| ) | ||
| AsyncImage( | ||
| model = uri, | ||
| modifier = Modifier | ||
| .align(Alignment.TopStart) | ||
| .offset { | ||
| IntOffset( | ||
| x = state.selectedOffset.x.toInt(), | ||
| y = state.selectedOffset.y.toInt(), | ||
| ) | ||
| } | ||
| .size(80.dp) | ||
| .graphicsLayer { | ||
| scaleX = 1.2f | ||
| scaleY = 1.2f | ||
| rotationZ = -4f | ||
| }, | ||
| contentScale = ContentScale.Crop, | ||
| contentDescription = null, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Overlay content disappears instantly during the exit animation.
When OnLongClickCancel fires, state.selectedLongClickImage becomes null before the AnimatedVisibility fadeOut completes. Because selectedLongClickImage?.let { uri -> ... } re-evaluates at every recomposition, Icon and AsyncImage are removed immediately — while the translucent backdrop is still fading out — producing a jarring visual where the image vanishes but the dimmed background lingers.
The standard fix is to keep a local "last non-null" snapshot so the overlay's content stays visible for the full duration of the exit animation.
✨ Proposed fix
+ var lastUri by remember { mutableStateOf<String?>(null) }
+ var lastOffset by remember { mutableStateOf(Offset.Zero) }
+ state.selectedLongClickImage?.let { img ->
+ lastUri = img
+ lastOffset = state.selectedOffset
+ }
+
AnimatedVisibility(
visible = state.selectedLongClickImage != null,
modifier = Modifier.zIndex(5f),
enter = fadeIn(),
exit = fadeOut(),
) {
Box(
modifier = Modifier
.fillMaxSize()
.background(Color.Black.copy(alpha = 0.7f))
.clickable(
interactionSource = remember { MutableInteractionSource() },
indication = null,
) {
state.eventSink(HomeUiEvent.OnLongClickCancel)
},
contentAlignment = Alignment.Center,
) {
- state.selectedLongClickImage?.let { uri ->
+ lastUri?.let { uri ->
Icon(
painter = painterResource(R.drawable.ic_image_share),
contentDescription = null,
tint = LightPink,
modifier = Modifier
.align(Alignment.TopStart)
.offset {
IntOffset(
- x = (state.selectedOffset.x + 20).toInt(),
- y = (state.selectedOffset.y - 120).toInt(),
+ x = (lastOffset.x + 20.dp.roundToPx()).toInt(),
+ y = (lastOffset.y - 120.dp.roundToPx()).toInt(),
)
}
.size(50.dp)
.clickable {
state.eventSink(HomeUiEvent.OnShareRelease(uri))
},
)
AsyncImage(
model = uri,
modifier = Modifier
.align(Alignment.TopStart)
.offset {
IntOffset(
- x = state.selectedOffset.x.toInt(),
- y = state.selectedOffset.y.toInt(),
+ x = lastOffset.x.toInt(),
+ y = lastOffset.y.toInt(),
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUi.kt`
around lines 118 - 155, The overlay content is removed immediately because
state.selectedLongClickImage is set to null before AnimatedVisibility's fadeOut
finishes; fix by introducing a local "last non-null" snapshot (e.g.,
lastSelectedLongClickImage) that you update when state.selectedLongClickImage
becomes non-null and continue to use that snapshot instead of
state.selectedLongClickImage inside the let block that renders Icon and
AsyncImage; clear the snapshot only after the exit animation completes (use
AnimatedVisibility's exit/onAnimationEnd callback or a LaunchedEffect keyed to
the visibility/animation) so the Icon and AsyncImage remain visible for the full
fadeOut when OnLongClickCancel fires.
| .offset { | ||
| IntOffset( | ||
| x = (state.selectedOffset.x + 20).toInt(), | ||
| y = (state.selectedOffset.y - 120).toInt(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Hardcoded pixel values break layout across screen densities.
The +20 and -120 inside the offset {} lambda are treated as raw pixels. Since selectedOffset comes from coordinates.positionInRoot() (already in pixels), these constants must also be converted from dp to pixels to remain consistent across densities — e.g., 120 px on an mdpi device equals 120dp, but only 40dp on an xxhdpi device.
The offset {} lambda is a Density.() -> IntOffset receiver, so dp → px conversion is available inline.
🐛 Proposed fix
- .offset {
- IntOffset(
- x = (state.selectedOffset.x + 20).toInt(),
- y = (state.selectedOffset.y - 120).toInt(),
- )
- }
+ .offset {
+ IntOffset(
+ x = (state.selectedOffset.x + 20.dp.roundToPx()).toInt(),
+ y = (state.selectedOffset.y - 120.dp.roundToPx()).toInt(),
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .offset { | |
| IntOffset( | |
| x = (state.selectedOffset.x + 20).toInt(), | |
| y = (state.selectedOffset.y - 120).toInt(), | |
| ) | |
| } | |
| .offset { | |
| IntOffset( | |
| x = (state.selectedOffset.x + 20.dp.roundToPx()).toInt(), | |
| y = (state.selectedOffset.y - 120.dp.roundToPx()).toInt(), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUi.kt`
around lines 125 - 130, The offset lambda is using hardcoded pixel constants
(+20, -120) which breaks across screen densities; change those raw numbers to
dp-to-px conversions using the Density receiver available in the offset { }
lambda (e.g., call 20.dp.toPx() and 120.dp.toPx()) before converting to Int for
IntOffset so the offsets are consistent with state.selectedOffset (which comes
from coordinates.positionInRoot()) and then use (x = (state.selectedOffset.x +
convertedX).toInt(), y = (state.selectedOffset.y - convertedY).toInt()) inside
the offset block.
| @Immutable | ||
| sealed interface HomeSideEffect { | ||
| data class ShareImage( | ||
| val uriString: String, | ||
| ) : HomeSideEffect | ||
| } |
There was a problem hiding this comment.
ShareImage lacks a unique key — consecutive shares of the same image won't re-trigger the side effect.
Other side effects in this PR (e.g., GraphSideEffect.ShowToast, NLSearchSideEffect.ShowToast) include a private val key: String = UUID.randomUUID().toString() to ensure each emission is treated as a distinct state change. Without it, calling ShareImage with the same uriString twice consecutively produces an equal object, so Compose won't detect a state change and the side effect won't fire again.
Proposed fix
`@Immutable`
sealed interface HomeSideEffect {
data class ShareImage(
val uriString: String,
+ private val key: String = java.util.UUID.randomUUID().toString(),
) : HomeSideEffect
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Immutable | |
| sealed interface HomeSideEffect { | |
| data class ShareImage( | |
| val uriString: String, | |
| ) : HomeSideEffect | |
| } | |
| `@Immutable` | |
| sealed interface HomeSideEffect { | |
| data class ShareImage( | |
| val uriString: String, | |
| private val key: String = java.util.UUID.randomUUID().toString(), | |
| ) : HomeSideEffect | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/home/src/main/java/com/metasearch/android/feature/home/HomeUiState.kt`
around lines 25 - 30, HomeSideEffect.ShareImage currently lacks a uniqueness key
so emitting the same uriString twice produces an equal object and Compose won't
re-trigger the side effect; update the ShareImage data class inside
HomeSideEffect to include a private val key: String =
UUID.randomUUID().toString() (import java.util.UUID as needed) so each emission
is distinct while keeping uriString accessible.
| android:width="512dp" | ||
| android:height="512dp" |
There was a problem hiding this comment.
Intrinsic size of 512dp is atypically large for an icon vector drawable.
The viewport is 24×24 but the declared width/height is 512dp. If this drawable is used in any context that respects intrinsic dimensions (e.g., wrap_content, Modifier.wrapContentSize(), or a legacy ImageView without explicit sizing), it will occupy 512dp. Convention for material icons is to match dimensions to the viewport — 24dp × 24dp.
🖼️ Proposed fix
<vector xmlns:android="http://schemas.android.com/apk/res/android"
- android:width="512dp"
- android:height="512dp"
+ android:width="24dp"
+ android:height="24dp"
android:viewportWidth="24"
android:viewportHeight="24">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| android:width="512dp" | |
| android:height="512dp" | |
| <vector xmlns:android="http://schemas.android.com/apk/res/android" | |
| android:width="24dp" | |
| android:height="24dp" | |
| android:viewportWidth="24" | |
| android:viewportHeight="24"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@feature/home/src/main/res/drawable/ic_image_share.xml` around lines 2 - 3,
The drawable's intrinsic size is incorrectly large: replace the android:width
and android:height attributes set to "512dp" with the correct icon size "24dp"
so the vector's intrinsic dimensions match its viewport (viewportWidth="24" and
viewportHeight="24"); update the attributes in ic_image_share.xml to
android:width="24dp" and android:height="24dp" to prevent the drawable from
occupying 512dp when used with wrap_content or default ImageView behavior.
| MetaSearchToast( | ||
| isVisible = toastMessage != null, | ||
| message = toastMessage ?: "", | ||
| ) |
There was a problem hiding this comment.
Pass toastMessage directly instead of toastMessage ?: "".
MetaSearchToast.message is declared String? = null. Passing toastMessage ?: "" means an empty (non-null) string reaches message?.let { Text(text = it, ...) } during the exit animation, which will try to render a Text with an empty string rather than skipping the block.
🛠️ Proposed fix
MetaSearchToast(
isVisible = toastMessage != null,
- message = toastMessage ?: "",
+ message = toastMessage,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MetaSearchToast( | |
| isVisible = toastMessage != null, | |
| message = toastMessage ?: "", | |
| ) | |
| MetaSearchToast( | |
| isVisible = toastMessage != null, | |
| message = toastMessage, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/main/src/main/java/com/metasearch/android/feature/main/MainActivity.kt`
around lines 106 - 109, The toast currently passes a non-null empty string via
MetaSearchToast(message = toastMessage ?: ""), causing the message?.let { ... }
block in MetaSearchToast to run during exit animation; change the call to pass
the nullable value directly (message = toastMessage) so MetaSearchToast.message
(String? = null) can be null and the let block will be skipped when there is no
message.
| var showDeleteDialog by remember { mutableStateOf(false) } | ||
| var pendingDeletePersonId by remember { mutableStateOf<Long?>(null) } | ||
| var pendingDeletePersonName by remember { mutableStateOf("") } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.kt | sed -n '45,65p'Repository: komodgn/meta-android
Length of output: 1034
🏁 Script executed:
# Search for all occurrences of these state variables in the file
rg -n "showDeleteDialog|pendingDeletePersonId|pendingDeletePersonName" feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.kt -B 2 -A 2Repository: komodgn/meta-android
Length of output: 2090
🏁 Script executed:
# Check git history for these specific variables to see if they changed from rememberRetained to remember
cd feature/person/src/main/java/com/metasearch/android/feature/person/ && git log -p --follow -- PersonPresenter.kt | grep -A 10 -B 10 "rememberRetained\|showDeleteDialog" | head -100Repository: komodgn/meta-android
Length of output: 46
🏁 Script executed:
# Search for any use of rememberRetained in the codebase to understand the pattern
rg -n "rememberRetained" --type=kt -B 1 -A 1 | head -50Repository: komodgn/meta-android
Length of output: 90
🏁 Script executed:
# Search for rememberRetained without type restriction
rg -n "rememberRetained" -B 2 -A 2Repository: komodgn/meta-android
Length of output: 10274
🏁 Script executed:
# Check the full git history of this file with more detail
git log --oneline feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.kt | head -20Repository: komodgn/meta-android
Length of output: 143
🏁 Script executed:
# Get a specific diff of changes to the delete state variables in git history
git log -p feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.kt | grep -B 20 -A 5 "showDeleteDialog\|rememberRetained" | head -150Repository: komodgn/meta-android
Length of output: 46
🏁 Script executed:
# Check the entire PersonPresenter.kt file for any comments about state retention
cat feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.ktRepository: komodgn/meta-android
Length of output: 5411
Use rememberRetained for delete dialog state to match codebase patterns and preserve dialog across configuration changes.
The delete dialog states (showDeleteDialog, pendingDeletePersonId, pendingDeletePersonName) should use rememberRetained instead of remember. They currently reset on screen rotation, which dismisses the confirmation dialog and leaves pendingDeletePersonId stale—an inconsistent UX.
This contradicts the established pattern in the codebase:
- Same file uses
rememberRetainedfor the search input (line 42) PersonDetailPresenterusesrememberRetainedfor all dialog states:showEditDialog,editName,editPhone,showMergeConfirmDialog,showPhotoSelectDialog- Other presenters consistently use
rememberRetainedfor dialog and UI state
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/person/src/main/java/com/metasearch/android/feature/person/PersonPresenter.kt`
around lines 53 - 55, Replace the three delete-dialog state properties that
currently use remember with rememberRetained so the confirmation dialog and
pending values persist across configuration changes: update showDeleteDialog,
pendingDeletePersonId and pendingDeletePersonName to use rememberRetained
instead of remember (matching the existing usage for the search input and other
dialog state in PersonDetailPresenter) to prevent the dialog from dismissing and
the id/name from becoming stale on rotation.
| LaunchedEffect(state.sideEffect) { | ||
| when (state.sideEffect) { | ||
| is PersonSideEffect.ShowToast -> { | ||
| EventHandler.sendEvent( | ||
| MetaSearchEvent.ShowToast( | ||
| message = state.sideEffect.message.asString(context), | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| else -> {} | ||
| } | ||
|
|
||
| if (state.sideEffect != null) { | ||
| eventSink(PersonUiEvent.InitSideEffect) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer null -> {} over else -> {} to preserve exhaustiveness checking.
Using else -> {} as a catch-all silently swallows any future side-effect types added to the sealed interface. Contrast this with NLSearchSideEffect.kt (from the relevant snippets) which uses null -> {} — that approach lets the Kotlin compiler warn when a new sealed subtype is unhandled. This inconsistency exists across several side-effect handlers in this PR (Person, Graph, FocusingSearch use else; NLSearch uses null).
Proposed fix
when (state.sideEffect) {
is PersonSideEffect.ShowToast -> {
EventHandler.sendEvent(
MetaSearchEvent.ShowToast(
message = state.sideEffect.message.asString(context),
),
)
}
- else -> {}
+ null -> {}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LaunchedEffect(state.sideEffect) { | |
| when (state.sideEffect) { | |
| is PersonSideEffect.ShowToast -> { | |
| EventHandler.sendEvent( | |
| MetaSearchEvent.ShowToast( | |
| message = state.sideEffect.message.asString(context), | |
| ), | |
| ) | |
| } | |
| else -> {} | |
| } | |
| if (state.sideEffect != null) { | |
| eventSink(PersonUiEvent.InitSideEffect) | |
| } | |
| } | |
| LaunchedEffect(state.sideEffect) { | |
| when (state.sideEffect) { | |
| is PersonSideEffect.ShowToast -> { | |
| EventHandler.sendEvent( | |
| MetaSearchEvent.ShowToast( | |
| message = state.sideEffect.message.asString(context), | |
| ), | |
| ) | |
| } | |
| null -> {} | |
| } | |
| if (state.sideEffect != null) { | |
| eventSink(PersonUiEvent.InitSideEffect) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/person/src/main/java/com/metasearch/android/feature/person/PersonSideEffect.kt`
around lines 16 - 32, The when on state.sideEffect in LaunchedEffect uses an
else branch which hides unhandled new sealed subtypes; change the catch-all from
else -> {} to null -> {} so the Kotlin compiler can enforce exhaustiveness for
the sealed interface (update the when handling around
LaunchedEffect(state.sideEffect), keeping the PersonSideEffect.ShowToast branch
and the subsequent eventSink(PersonUiEvent.InitSideEffect) logic intact). Ensure
any other similar handlers (e.g., in Graph/FocusingSearch) follow the same null
-> {} pattern.
Summary by CodeRabbit
Release Notes
New Features
Improvements